Use selectors lib to address select() fd num limit #301
Conversation
tommilligan
force-pushed
the
reproduce-select-out-of-range
branch
2 times, most recently
from
f626070 to
df4477c
Compare
tommilligan
force-pushed
the
reproduce-select-out-of-range
branch
2 times, most recently
from
f5fe725 to
e646d34
Compare
cheroot/test/test_server.py
Outdated
| import resource | ||
|
|
||
| # Get current resource limits to restore them later | ||
| (soft_limit, hard_limit) = resource.getrlimit(resource.RLIMIT_NOFILE) | ||
| increased_nofile_limit = 2048 |
There was a problem hiding this comment.
It may be a good idea to isolate the resource manipulations into a fixture.
There was a problem hiding this comment.
I would say this is only required if we want to reuse the manipulation. As is I feel this would probably not be worth the effort, especially combined with the conditional import of the resource module
There was a problem hiding this comment.
It's not only about reusing things. Dividing the code into small pieces makes it more easily readable and maintainable. It also helps to have cleaner abstractions w/o leaks...
cheroot/connections.py
Outdated
| conn = server_socket | ||
| finally: | ||
| for sock in socket_dict: | ||
| self._selector.unregister(sock) |
There was a problem hiding this comment.
I'm not yet familiar with selectors, could you please tell me what happens with sockets between unregister() and the following register() at some point in time? Since the previous major contribution caused serious regressions, I'd like to be extra careful now...
There was a problem hiding this comment.
I'm not quite sure what you're asking here - are you saying we should be tracking events from sockets throughout their whole lifetime? i.e. as a connection is added to self.connections, we begin listening for events on it, and then as it's removed, we unregister it?
I haven't used sockets a lot before, so guidance/another solution from anyone else is very welcome
There was a problem hiding this comment.
I was just wondering how things work because I haven't used this API before. But that's fine if you don't have a detailed answer.
|
@tommilligan thanks for the PR! Now that I see that it also tries to provide a bugfix, the title and the description should be adjusted accordingly. |
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
…ptors`" This reverts commit 7e0900c.
This change addresses a bug in selectors2 (that is resolved in master
but is unreleased) with fd of the "long" type not being recognized as
"int". Here's the traceback it causes:
Traceback (most recent call last):
File "C:\projects\cheroot\cheroot\server.py", line 1788, in serve
self.tick()
File "C:\projects\cheroot\cheroot\server.py", line 2023, in tick
conn = self.connections.get_conn(self.socket)
File "C:\projects\cheroot\cheroot\connections.py", line 163, in get_conn
self._selector.unregister(fno)
File "c:\projects\cheroot\.tox\python\lib\site-packages\selectors2.py", line 249, in unregister
key = super(SelectSelector, self).unregister(fileobj)
File "c:\projects\cheroot\.tox\python\lib\site-packages\selectors2.py", line 155, in unregister
key = self._fd_to_key.pop(self._fileobj_lookup(fileobj))
File "c:\projects\cheroot\.tox\python\lib\site-packages\selectors2.py", line 127, in _fileobj_lookup
return _fileobj_to_fd(fileobj)
File "c:\projects\cheroot\.tox\python\lib\site-packages\selectors2.py", line 90, in _fileobj_to_fd
raise ValueError("Invalid file object: {0!r}".format(fileobj))
ValueError: Invalid file object: 916L
Refs:
* sethmlarson/selectors2#10
* https://twitter.com/webKnjaZ/status/1285695028136992769
* https://github.com/berkerpeksag/selectors34/blob/1.2.0/selectors34.py#L51
* cherrypy#301 (comment)
This reverts commit e996f20. Thanks to @sethmlarson who published `selectors==2.0.2` on PyPI we no longer need a fallback to `selectors34`. Ref: https://twitter.com/sethmlarson/status/1285706586514751489
webknjaz
force-pushed
the
reproduce-select-out-of-range
branch
from
12e08a6 to
dc2c17d
Compare
This comment has been minimized.
This comment has been minimized.
This reverts commit 72252db.
|
So I'm going to merge this today/tomorrow if nobody will object by then. This suggestion #301 (comment) seems desirable but is not a blocker so if @tommilligan will choose not to implement it or do it in a separate PR, it's fine. |
This comment has been minimized.
This comment has been minimized.
|
Thanks, @tommilligan! I released this as v8.4.0: https://pypi.org/project/cheroot/8.4.0/. P.S. Feel free to contribute a follow-up addressing https://github.com/cherrypy/cheroot/pull/301/files#r458248844. |
|
@webknjaz thanks for all your help on this, and getting a new release out so promptly! I like the suggestion of refactoring how we manage connections a bit so that the use of |
|
Great, thanks! I've filed an issue for the refactoring: #304. |
|
Thanks @tommilligan for working on this. I'll test it out on my setup. |
|
FTR the socket allocation fixture seems to be flaky under PyPy: > assert sock.fileno() >= resource_limit
E assert 24 >= 2048
E + where 24 = <bound method _socketobject.fileno of <socket._socketobject object at 0x000000000a9037c0>>()
E + where <bound method _socketobject.fileno of <socket._socketobject object at 0x000000000a9037c0>> = <socket._socketobject object at 0x000000000a9037c0>.fileno(https://travis-ci.com/github/cherrypy/cheroot/jobs/365058505#L543) It'd be interesting to explore why it is different there... |
|
Tested the fix with my application. Fix seems to be working. Thanks @@tommilligan @webknjaz! |
❓ What kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)Fixes #249
❓ What is the current behavior? (You can also link to an open issue here)
HTTPServererrors if it tries to open a filedescriptor larger than1024.❓ What is the new behavior (if this is a feature change)?
A failing test
test_high_number_of_file_descriptorshas been added to capture this error.The call to
selecthas been replaced with theselectors/selectors2module.📋 Other information:
📋 Checklist:
and description in grammatically correct, complete sentences
This change is